AutoMigrate for Postgres dialect #926

bevzzz · 2023-11-01T19:29:20Z

This PR is my second attempt at implementing #456 :)

This time around I decided not to meddle with ALTER TABLE query and focus on the functionality we want to bring -- AutoMigrator. I took into account our discussions in #726 and left it up to the dialects to implement "alter table" operations, keeping it internal. My thinking was that, if auto migration is done well, the users wouldn't need to work with ALTER TABLE all that much.

AutoMigrator exposes a simple and minimal API: CreateSQLMigrations(ctx) and Migrate(ctx, options). These are similar to Migrator except that they do not require writing any SQL/Go migrations manually.

What AutoMigrator can do:

Create, drop, and rename tables
Create, drop, and rename column
Modify DEFAULT value
Modify NOT NULL and UNIQUE constraints
Modify primary key and foreign key constraints
Change column data type
Add or remove identity
Create SQL migration files with support for transactional migrations (.tx.up.sql/.tx.down.sql)

What it doesn't do:
AutoMigrator performs schema migrations and not data migrations. For example, when changing a primary key on the table, related foreign keys will not be updated -- they will continue referencing the old PK; to "re-link" them would require updating the referencing values themselves, which is beyond AutoMigrator's scope.

Presently, only pgdialect supports database inspection and migration, for which it implements two additional interfaces. AutoMigrator will return an informative error if used with a dialect that doesn't support this feature.

Other changes:

test.sh forwards additional arguments to go test
(for example, now possible to run 1 test with ./test.sh -run=TestAutoMigrator_Run)
added Schema string field to differentiate tables in different schemas.
each Dialect now reports the name of the default schema
had to update commit message in 95c4a8e because it was upsetting the linter

In-code documentation
Documentation on the website
Example CLI to generate and run auto-migrations

bevzzz · 2023-11-01T19:35:17Z

I had to update a lot of to the existing unit tests (~5-7 files) because I noticed that many of them weren't properly cleaning up the database, causing side-effects in the auto-migrate tests.
All of those changes come in a single commit and I could open a different PR for them to be merged separately.

UPD: opened #927 for this specific change. Should make it easier to review the current one once the other changes are merged.

…ce#1028) * feat(schema): add support type for net/netip.Addr and net/netip.Prefix * fix(schema): net.IPNet(not ptr) is not implement fmt.Stringer Edit: updated commit message to comply with commitlint [subject-case] rule. Original subject: "Add support type..."

- RawQuery - CreateTableQuery

Additionally: - allow custom FK names (limited applicability rn) - implement GetReverse for AddFK and DropFK - detect renamed foreign keys - EqualSignature handles empty models (no columns)

This is a WIP commit.

- Do not implement AppendQuery on Operation level. This is a leaky abstraction as queries are dialect-specific and migrate package should not be concerned with how they are constructed. - AlterTableQuery also is an unnecessary abstraction. Now pgdialect will just build a simple string-query for each Operation. - Moved operations.go to migrate/ package and deleted alt/ package. - Minor clean-ups and documentation. testChangeColumnType is commented out because the implementation is missing.

Bufixes and improvements: - pgdialect.Inspector canonicalizes all default expressions (lowercase) to make sure they are always comparable with the model definition. - sqlschema.SchemaInspector canonicalizes all default expressions (lowercase) - pgdialect and sqlschema now support type-equivalence, which prevents unnecessary migrations like CHAR -> CHARACTER from being created. Changing PRIMARY KEY and UNIQUE-ness are outside of this commit's scope, because those constraints can span multiple columns.

vmihailenco · 2024-11-09T11:52:09Z

@bevzzz few things that I've noticed:

You've added schema.FQN to support schema.table names. Do we really need it? It complicates things a lot and I don't think that we should support more than a single schema, e.g. do we really plan to support moving tables between schemas? Is that even possible?

I suggest we drop schema.FQN and ignore the schema name althogether.
In every interface you have Tables []Table, but Tables map[string]Table in the implementation because it is handy. That will cause some issues, because maps don't preserve the order. I suggested we use ordered maps in all such places so we have both map and list, for example, https://github.com/wk8/go-ordered-map/v2

So we will have:

type Table interface {
  GetTables() *orderedmap.Map[string, Table]
}

I also added some clarification comment to BaseTable, BaseColumn etc so they must not be used outside dialects

// BaseTable is a base table definition.
//
// Dialects and only dialects can use it to implement the Table interface.
// Other packages must use the Table interface.

WDYT?

bevzzz · 2024-11-09T14:56:36Z

do we really plan to support moving tables between schemas? Is that even possible?

No, I don't think moving schemas between tables is possible. Rather, I wanted to make sure that the tables in different schemas aren't shadowing each other names, i.e. I want to be able to rename books.authors -> writers without renaming public.authors or paintings.authors.

Most databases I've worked with had several schemas, so I thought this may make automigrations more resilient to name clashes.

UPD: I realize now that we can achieve the same by using the name from the bun:"table" tag directly. Will remove schema.FQN 👌

That will cause some issues, because maps don't preserve the order. I suggested we use ordered maps

Sure, I would even argue that GetTables may return []Table and have detector use orderedmap.Map internally, where it's most needed. This way dialects needn't bring orderedmap as a dependency.

Other than the different order in which Operations may be applied, is there any reason a lack of order in map may cause issues? I'm asking because the dependencies between operations are always resolved during migration planning and the order in which tables/columns are supplied does not break that.

vmihailenco · 2024-11-10T10:57:52Z

is there any reason a lack of order in map may cause issues?

I assume that query generation won't be stable if 2 columns/tables are changed at once.

I also see that the tests rely on having a map[string]Table/Column. Also I had a similar task before and orderedmap worked well.

vmihailenco · 2024-11-10T10:59:57Z

@bevzzz I am going to introduce orderedmap myself and leave FQN removal to you. Hopefully that's okay with you.

Regarding schemas, I think that we might need to accept a schema name via configuration and then only work with that schema. But that can be added later.

vmihailenco · 2024-11-10T12:44:26Z

@bevzzz я все :)

Тесты я поправил но билд почему-то не проходит c непонятной ошибкой. Предлагаю пока это игнорировать.

AutoMigrator only supports 1 schema at a time. Use WithSchemaName() option to configure it. Defaults to the default schema in the dialect.

Now that AutoMigrator only works with one schema at a time, there's no need to keep the code which was used ot differentiate tables between schemas

bevzzz · 2024-11-11T21:15:39Z

Short update: we've decided to drop multi-schema support, so the user would instead configure AutoMigrator to work with a particular schema by passing migrate.WithSchemaName("my_schema") option.
By default, AutoMigrator will continue to use the dialect's default schema, e.g. public for Postgres.

vmihailenco · 2024-11-12T13:39:18Z

Looks like we can now remove (Dialect) DefaultSchema(). Overall this looks pretty good.

migrate/sqlschema/inspector.go

vmihailenco · 2024-11-12T13:44:48Z

This looks pretty good. @bevzzz you should have necessary permissions to merge this so feel free to do it.

bevzzz · 2024-11-12T16:23:32Z

@vmihailenco sorry, I've completely overlooked the last suggested changes. Will fix them in a follow-up PR, if that's alright with you.

Follow-up #926

bevzzz changed the title ~~AutoMigrate: Rename table~~ AutoMigrate: Rename/Create/Drop tables Nov 3, 2023

bevzzz mentioned this pull request Nov 3, 2023

Cleanup test databases to avoid side-effects #927

Merged

bevzzz force-pushed the feature/automigrate-rename-table branch 5 times, most recently from 3ef6553 to ce75416 Compare August 3, 2024 14:46

bevzzz force-pushed the feature/automigrate-rename-table branch 3 times, most recently from eeadd41 to 9faa55b Compare October 20, 2024 10:00

bevzzz force-pushed the feature/automigrate-rename-table branch from 85d0877 to e7e7c4f Compare October 27, 2024 11:18

Aoang and others added 18 commits October 27, 2024 12:24

test: allow passing go test flags to test.sh

e04de83

feat: detect renamed tables

8857bab

feat: implement fmt.Stringer queries

5060e47

- RawQuery - CreateTableQuery

test: cleanup test databases to avoid side-effects

6dfdf95

feat(automigrate): detect renamed tables

c03938f

chore: re-organize packages

27a5a2b

feat: use *bun.DB in MigratorDialect

a8788bf

chore: re-organize packages

75a6f2b

feat: detect Create/Drop table

408859f

feat: detect modified relations

a918dc4

feat: migrate FKs

4c1dfdb

feat: improve FK handling

a822fc5

Additionally: - allow custom FK names (limited applicability rn) - implement GetReverse for AddFK and DropFK - detect renamed foreign keys - EqualSignature handles empty models (no columns)

feat: detect renamed columns

886d0a5

chore: resolve dependencies

1322895

This is a WIP commit.

fix: ignore case for type equivalence

c3253a5

vmihailenco added 2 commits November 9, 2024 13:23

fix most tests

8af7563

rename Schema to Database

53eabcf

vmihailenco added 3 commits November 10, 2024 14:29

add orderedmap

a08c009

remove go 1.22

9abdfca

add tz

30dc4db

bevzzz added 4 commits November 10, 2024 19:43

refactor: move FQN to sqlschema package

6b0ddc1

fix: remove schema name from t.Name during bun-schema inspection

31ed582

refactor: drop multi-schema support

4e96659

AutoMigrator only supports 1 schema at a time. Use WithSchemaName() option to configure it. Defaults to the default schema in the dialect.

refactor: retire sqlschema.FQN

48811e0

Now that AutoMigrator only works with one schema at a time, there's no need to keep the code which was used ot differentiate tables between schemas

bevzzz added 2 commits November 12, 2024 09:49

test: activate commented-out tests

1799a0f

feat: include target schema name in migration name

ac8d221

vmihailenco reviewed Nov 12, 2024

View reviewed changes

migrate/sqlschema/inspector.go Show resolved Hide resolved

vmihailenco reviewed Nov 12, 2024

View reviewed changes

migrate/sqlschema/inspector.go Show resolved Hide resolved

vmihailenco reviewed Nov 12, 2024

View reviewed changes

migrate/sqlschema/inspector.go Show resolved Hide resolved

vmihailenco reviewed Nov 12, 2024

View reviewed changes

migrate/sqlschema/inspector.go Show resolved Hide resolved

vmihailenco approved these changes Nov 12, 2024

View reviewed changes

bevzzz merged commit b10bd39 into uptrace:master Nov 12, 2024
2 of 3 checks passed

bevzzz mentioned this pull request Nov 13, 2024

Follow-up #926 #1058

Merged

vmihailenco added a commit that referenced this pull request Nov 13, 2024

Merge pull request #1058 from bevzzz/feature/automigrate-followup

abcd779

Follow-up #926

capcom6 mentioned this pull request Nov 15, 2024

MariaDB: add support for RETURNING clause in DELETE statements #1059

Merged

bevzzz deleted the feature/automigrate-rename-table branch November 25, 2024 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AutoMigrate for Postgres dialect #926

AutoMigrate for Postgres dialect #926

bevzzz commented Nov 1, 2023 •

edited

Loading

bevzzz commented Nov 1, 2023 •

edited

Loading

vmihailenco commented Nov 9, 2024 •

edited

Loading

bevzzz commented Nov 9, 2024 •

edited

Loading

vmihailenco commented Nov 10, 2024

vmihailenco commented Nov 10, 2024

vmihailenco commented Nov 10, 2024

bevzzz commented Nov 11, 2024

vmihailenco commented Nov 12, 2024

vmihailenco commented Nov 12, 2024

bevzzz commented Nov 12, 2024

AutoMigrate for Postgres dialect #926

AutoMigrate for Postgres dialect #926

Conversation

bevzzz commented Nov 1, 2023 • edited Loading

bevzzz commented Nov 1, 2023 • edited Loading

vmihailenco commented Nov 9, 2024 • edited Loading

bevzzz commented Nov 9, 2024 • edited Loading

vmihailenco commented Nov 10, 2024

vmihailenco commented Nov 10, 2024

vmihailenco commented Nov 10, 2024

bevzzz commented Nov 11, 2024

vmihailenco commented Nov 12, 2024

vmihailenco commented Nov 12, 2024

bevzzz commented Nov 12, 2024

bevzzz commented Nov 1, 2023 •

edited

Loading

bevzzz commented Nov 1, 2023 •

edited

Loading

vmihailenco commented Nov 9, 2024 •

edited

Loading

bevzzz commented Nov 9, 2024 •

edited

Loading